feat: Public Actor Permissions #2053
Conversation
tobice
left a comment
tobice
left a comment
There was a problem hiding this comment.
Gave it a quick look; it still needs a bit of love. I'd even rethink the structure a bit ;D
sources/platform/actors/development/actor_definition/input_schema/specification.md
Outdated
Show resolved
Hide resolved
| - `["READ"]` — the Actor may read from the referenced resource(s). | ||
| - `["READ", "WRITE"]` — the Actor may read and write to the referenced resource(s). | ||
|
|
||
| Notes: |
There was a problem hiding this comment.
Let's mention that this will be communicated to the users via the form (the tooltip).
There was a problem hiding this comment.
This is the copy that needs to be updated. Let's not forget about updating this screenshot as well.
…y-docs into feat/public-actor-permissions
|
Preview for this PR was built for commit |
raethlo
requested review from
tobice
and removed request for
fnesveda and
janbuchar
|
Preview for this PR was built for commit |
nmanerikar
left a comment
nmanerikar
left a comment
There was a problem hiding this comment.
A few comments, suggestions and corrections.
sources/platform/actors/development/permissions/migration_guide.md
Outdated
Show resolved
Hide resolved
sources/platform/actors/development/permissions/migration_guide.md
Outdated
Show resolved
Hide resolved
sources/platform/actors/development/permissions/migration_guide.md
Outdated
Show resolved
Hide resolved
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
|
🚧 Note: before this can go live we still need to update the screenshots. |
|
Preview for this PR was built for commit |
nmanerikar
left a comment
nmanerikar
left a comment
There was a problem hiding this comment.
Thanks for the changes! I just have a couple more observations. Apart from those, this looks fine from my perspective.
sources/platform/actors/development/permissions/migration_guide.md
Outdated
Show resolved
Hide resolved
|
Preview for this PR was built for commit |
danpoletaev
left a comment
danpoletaev
left a comment
There was a problem hiding this comment.
Looks good 👍
Left few suggestions, more like nits and optional stuff, that might improve readability.
Main concern is that currently permissions and migration_guide a bit mixed up, there're some section that might be more logical in migration_guide, but it is only my opinion, we can discuss it 🙃
|
|
||
| See the full [input schema reference for details.](../actor_definition/input_schema/specification.md). | ||
|
|
||
| ### Requesting full permissions |
There was a problem hiding this comment.
Title doesn't correspond to what we write in the paragraph. Who is requesting full permissions? Sounds confusing.
Maybe Keeping full permissions?
There was a problem hiding this comment.
Also not sure, maybe move to migrations?
|
|
||
| Recommended minimum SDK versions: | ||
|
|
||
| - JavaScript SDK: [[email protected]](https://github.com/apify/apify-sdk-js/releases/tag/apify%403.4.4) |
There was a problem hiding this comment.
(nit): why versions are inconsistent, let's just keep numbers ? 3.4.4 and 3.0.0
| ::: | ||
|
|
||
|
|
||
| ### Accessing user provided storages |
There was a problem hiding this comment.
(opt): This section feels strange for me. In the article we're talking about how Actor permissions work and than in this section we switch to just limited permissions, also we kind of repeat ourselves a bit, because there's a very similar information in the migration, maybe this section should be in migration?
But on the other hand this section would make sense for me, if we'll also speak about how it currently works for full permissions:
- no need to pass resourcePermissions
- some devs could avoid using inputs and just have it hardcoded in the code.
There was a problem hiding this comment.
Got it, I've added a bit about full permissions here b6d6eb8
|
|
||
| --- | ||
|
|
||
| Use this guide to migrate existing Actors to use [limited permissions](index.md#how-actor-permissions-work). The general prerequisite is to update to the latest [Apify SDK](https://docs.apify.com/sdk). |
There was a problem hiding this comment.
(nit):
| Use this guide to migrate existing Actors to use [limited permissions](index.md#how-actor-permissions-work). The general prerequisite is to update to the latest [Apify SDK](https://docs.apify.com/sdk). | |
| This guide explains how to migrate your existing Actors to use [limited permissions](https://github.com/apify/apify-docs/pull/2053/index.md#how-actor-permissions-work). Before you start, make sure your Actor uses the latest [Apify SDK](https://docs.apify.com/sdk) | |
| . |
|
|
||
| Actors sometimes use named storages for caching or persisting state across runs. With limited permissions, an Actor can create a named storage on its first run and will automatically retain access to it in all subsequent runs by the same user. | ||
|
|
||
| If your Actor previously relied on accessing a pre-existing named storage, you will need to rename it in your code. This will cause the Actor to recreate the storage under the new system on its next run. |
There was a problem hiding this comment.
(nit): oh, I am afraid that no one would do it, it seems complicated 😄
But I understand that there's not much we can do about it.
Maybe I would also add to this section something like avoid using hardcoded storages and just update the input, so user can provide it(and link to "The actor accesses storages provided by users" or to current "Accessing user provided storages")?
There was a problem hiding this comment.
That's a pretty good idea how to migrate existing storages 👍🏻
|
👋 With the detailed review from Dan and Nish, I don't think my voice is needed here... so Roman don't wait on an approval from me 😊 |
Co-authored-by: Daniil Poletaev <[email protected]>
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
| ### Accessing user provided storages | ||
|
|
||
| Limited-permissions Actors can access storages that users explicitly provide via the Actor input. Use the input schema to add a storage picker and declare exactly which operations your Actor needs. | ||
| By default, limited-permissions Actors can't access users storages. However, they can access storages that users explicitly provide via the Actor input. To do so, use the input schema to add a storage picker and declare exactly which operations your Actor needs. |
There was a problem hiding this comment.
either user storages or users' storages. For consistency with other places, user storages - I see this used below.
|
Preview for this PR was built for commit |
nmanerikar
left a comment
nmanerikar
left a comment
There was a problem hiding this comment.
One final comment, and reminder to update the screenshots.
Other than that, this looks fine.
I assume there will be a review from the content team as well?
|
|
||
|  | ||
|
|
||
| Over time, the distinction between these permission levels will become more prominent. For example, Actors requiring full permissions may display an orange badge, appear lower in Store rankings, or show extra confirmation dialogs when you run them. Whenever possible, choose Actors that use **limited permissions**. They are safer, easier to trust, and sufficient for most workflows. |
There was a problem hiding this comment.
I missed this previously, but in keeping with @TC-MO's comment about dropping mentions of future changes, we should drop this part.
This PR adds docs for a new platfrom feature: Actor permissions.